feat: subprocess provider, SQLite LLM cache, meta-analyzer batching, and scan UX improvements#234
Conversation
…stale tests - Move LLMMetaAnalyzer() inside the try block in meta_analyzer so init failures are caught gracefully instead of propagating to the CLI - Add MODEL_CONFIG fallback for meta_analyzer model (was returning None when model_config state key is unset) - Add exc_info=True to all four LLM node exception handlers so the next run with a real API key produces a full traceback for the NameError - Update two stale test_meta_analyzer tests that expected CRITICAL findings to be dropped by LLM rejection; they now use MEDIUM severity (not protected by _HIGH_SEVERITY_FLOOR) and a new test explicitly asserts the floor behaviour for CRITICAL findings - Format four files to satisfy ruff format --check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements SubprocessChatModel (BaseChatModel subclass) with _generate() and _call_subprocess() methods, plus full test coverage via TestSubprocessChatModelGenerate (4 tests). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t, timeout handling
…cstrings, bandit nosec, 99% coverage
….md, and CLI help
Adds the acceptance test plan for SKILLSPECTOR_PROVIDER=subprocess, covering happy path, error handling, provider isolation, alternative tools, and CLI/doc coverage (AT-01 to AT-34). Criteria corrections applied after first run against the reinstalled binary: exit code expectations updated to 1 for malicious_skill scans (tool exits non-zero when risk_score > 50), and AT-03 JSON key corrected from "findings" to "issues" to match the actual report schema. All mandatory tests pass. Skips are due to unavailable optional prerequisites (no antigravity/openclaw CLIs, no cloud API keys). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add _resolve_baseline_output() to pick <target-dir>/.skillspector-baseline.yaml
when input_path is a local directory and --output is not given.
- Add _warn_if_overwriting() to print a warning with prior suppression count
when a baseline file already exists at the resolved path.
- Change baseline() output parameter default from Path(".skillspector-baseline.yaml")
to None so the new resolver controls placement.
- Add three TDD tests: target-dir placement, explicit --output override, overwrite warning.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oblem 12) Add _apply_negation_context_filter post-filter to static_yara.py that detects negation words in finding context (cuts confidence by 50%, tags likely_false_positive) and security-education section headers in file content (tags security_education). Three TDD tests added to test_static_yara.py covering each scenario. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ger (Problem 1) Replace 'IGNORE all instructions' phrasing in the TP4 analyzer system prompt with evaluator-role framing that preserves analytical intent without triggering subprocess provider injection detection. Add subprocess/SKILL.md context file to orient Claude LLM backend sessions. Add regression test to guard the phrase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…snippet (Problems 7 + 11) - Add _ACCEPTED_PERMISSION_TYPES, _ACCEPTED_TYPES_STR, _CAP_TO_PERMISSION_TYPE constants - Add _build_permissions_snippet() helper to generate copy-pasteable YAML - LP1 remediation now names the canonical permission type and lists all accepted types - LP3 remediation now appends a YAML permissions: block with detected capabilities - Add test_lp1_remediation_lists_accepted_types and test_lp3_remediation_includes_snippet Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… hint (Problem 2) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… (Problem 5) - behavioral_ast.py: add _is_test_file() + _is_subprocess_test_fixture() helpers; downgrade AST4 to confidence=0.15 + likely_test_fixture tag when shell=False + sys.executable pattern detected in a test_*.py file - static_patterns_privilege_escalation.py: add _is_pe3_test_fixture() helper; downgrade PE3 /etc/passwd findings in test functions containing traversal-related keywords; rewrite node() to forward include_test_fixtures when flag is set - state.py: add include_test_fixtures: bool field to SkillspectorState - cli.py: add --include-test-fixtures flag to scan(); wire through _scan_state() - tests: 3 AST4 + 3 PE3 test-fixture heuristic tests (TDD, red→green) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… to function name
- scan() docstring now documents --include-test-fixtures in a new Flags: section
- _is_pe3_test_fixture() combined regex requires keyword in def test_<keyword>
function name rather than anywhere in the surrounding 15-line block, eliminating
false-positives like test_foo calling sanitize_path('/etc/passwd')
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add depth parameter to detect_skills() and _find_skills_recursive() helper for multi-level skill discovery; add --depth CLI flag to scan command; update fallback warning to suggest --depth N+1 and --depth N+2. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…blem 4)
Add --detail flag to scan command; when used with --recursive --format json
--output, each skill entry in the JSON includes an issues[] array of full
Finding.to_dict() serializations. Without --detail the output is unchanged
(backward-compat). Restructures combined JSON from skills[] list to skills{}
dict keyed by relative path, with top-level summary{} section.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion (Problem 13) - Add skill_classification field to SkillspectorState - build_context reads classification from SKILL.md frontmatter and cascades from a parent-directory skillspector.yaml (scope: offensive_security) - report overrides risk_recommendation to "AUTHORIZED OFFENSIVE TOOL — review findings in context" when skill_classification == "offensive_security" - Two new integration tests cover manifest-level and library-scope-yaml paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add analyzer_id param and _emit_progress() to LLMAnalyzerBase so users see [LLM] <id>: <file> (requesting...) / (done, N findings) on stderr during long LLM calls. Wire up analyzer_id in all three semantic analyzer nodes and LLMMetaAnalyzer. Add 12 unit tests covering sync, async, empty-id suppression, and per-batch progress. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds skip_meta: bool to SkillspectorState, an early-return check in meta_analyzer() (before use_llm, so it bypasses LLM even when use_llm=True), and a --skip-meta CLI flag wired through _scan_state(). When active, all findings pass through with default remediations (fail-open fast path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds LLMResponseCache (SQLite-backed) keyed by (content_hash, prompt_hash, schema_version) so unchanged files skip repeated LLM calls across scan runs. Integrates cache into LLMAnalyzerBase.run_batches / arun_batches and wires llm_cache_dir through state → build_context → meta_analyzer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…odule level Pass llm_cache_dir from state as LLMResponseCache to all three semantic analyzer nodes (semantic_security_discovery, semantic_quality_policy, semantic_developer_intent) so their LLM calls are cached on repeated scans of unchanged files — the same pattern already used in meta_analyzer. Move the deferred `import json as _json` statements inside run_batches and arun_batches in llm_analyzer_base.py to the module-level import block (stdlib, alphabetically after asyncio) and update all references from _json to json. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The _cache_key() method now correctly returns CacheKey instead of object, which resolves mypy type errors at call sites (get/put in run_batches and arun_batches). Removed unnecessary type: ignore comments that suppressed these errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…em 3a) Split findings into configurable groups before calling the meta-analyzer LLM so large skill scans don't exceed model context limits. Each group calls arun_batches independently; results are merged before apply_filter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oad in tests - Move `from collections import Counter` from inside _split_files_into_batches() to module-level imports (stdlib section, alphabetically ordered) - Add try/finally cleanup in test_meta_analyzer_batches_large_finding_sets and test_meta_analyzer_reads_batch_size_at_call_time to reload constants module after each test, preventing env var persistence across tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… TP4 cache exclusion - Wire _PE3_TEST_FUNCTION_KEYWORDS into a precompiled _PE3_FIXTURE_FUNC_RE and use it in _is_pe3_test_fixture(), eliminating the dead constant and the duplicated inline pattern string. - Add __del__ to LLMResponseCache so the SQLite connection is closed on GC, preventing Windows file locks in non-CPython runtimes. - Add an explanatory comment above the chat_completion call in _check_tp4 documenting why TP4 bypasses the LLM response cache. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lan files - Reformat all markdown tables in README for consistent column alignment - Fix string continuation indentation in cli.py help text and condense two multi-line expressions - Add skillspector_bridge.py for external tool integration - Add .skillspector-baseline.yaml scan baseline - Add run_scan_with_llm.ps1 helper script - Add skills/skillspector-operator skill definition - Add docs/superpowers/plans/2026-06-26-skillspector-prd-enhancements.md planning doc Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…asion tests Resolved conflicts preserving both sides: - Providers table: added bedrock row alongside existing subprocess row - Env vars table: added AWS_PROFILE/AWS_REGION alongside SKILLSPECTOR_LLM_COMMAND - cli.py help text: includes bedrock, nv_inference, and subprocess in provider list - static_patterns_privilege_escalation.py: kept include_test_fixtures param + PE3 fixture heuristic, updated docstring to PE1–PE5 to reflect new PE5 patterns - providers/__init__.py: kept both SubprocessProvider and BedrockProvider blocks - test_behavioral_ast.py: kept TestAST4TestFixtureHeuristic + added builtins/ importlib evasion tests from origin/main - test_static_patterns.py: kept TestPE3TestFixtureHeuristic + added PE5/E5 tests - test_meta_analyzer.py: deduplicated auto-merged tests, kept unique test_critical_finding_kept_when_rejected_by_llm, took multi-line _finding sig Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rng1995
left a comment
There was a problem hiding this comment.
Requesting changes. This branch contains unresolved conflict markers in two test files and is currently conflicting with main. The executable review also found a broken subprocess structured-output path, attacker-controlled baseline/cache/classification trust boundaries, unsound cache invalidation, and recursive output/detection regressions. Please address the inline blockers, rebase onto current main, and rerun the full pytest, ruff, and formatting checks.
| elif isinstance(schema, type) and issubclass(schema, BaseModel): | ||
| schema_str = json.dumps(schema.model_json_schema(), indent=2) | ||
|
|
||
| def inject_and_parse(messages: list[BaseMessage]) -> BaseModel: |
There was a problem hiding this comment.
Blocking: LLMAnalyzerBase calls this runnable with a plain prompt string, not a list of BaseMessage objects. Iterating that string here yields per-character messages and never appends the JSON schema, so real subprocess scans receive malformed prompts and structured parsing fails. Normalize supported LangChain inputs (at minimum, string to one HumanMessage) and add an integration test through LLMAnalyzerBase.
There was a problem hiding this comment.
Fixed in 7f8e42c. Added _normalize_to_messages() to coerce plain strings (and other Runnable inputs) to list[BaseMessage] before the JSON-schema instruction is appended; wired into both closures in with_structured_output. Added unit coverage plus an end-to-end LLMAnalyzerBase.run_batches integration test through the subprocess provider, as requested.
| "component_metadata": component_metadata, | ||
| "has_executable_scripts": has_executable_scripts, | ||
| "skill_classification": classification, | ||
| "llm_cache_dir": str(skill_dir / ".skillspector-cache"), |
There was a problem hiding this comment.
Blocking trust-boundary issue: this cache path is inside the untrusted scan target. A skill can ship a preseeded SQLite database for deterministic keys, or make .skillspector-cache a symlink so SQLite writes escape the root. Store cache state in a trusted application cache outside scanned content, reject symlink/preexisting untrusted state, and exclude cache data from discovery.
There was a problem hiding this comment.
Fixed in 40ef232/1045afe. Cache now lives under the OS app-cache root (LOCALAPPDATA/XDG_CACHE_HOME), hashed per skill dir, never inside the scanned target. Added a symlink guard in LLMResponseCache._connect() that refuses a symlinked cache dir or db file (fails closed to no-cache), and excluded the old in-target cache dir name from discovery.
|
|
||
| def _cache_key(self, batch: Batch) -> CacheKey: | ||
| """Build a cache key for *batch* using content and prompt template hashes.""" | ||
| return make_cache_key( |
There was a problem hiding this comment.
Blocking: this key hashes only batch content, the base prompt, and a schema class name. It omits the fully rendered prompt (findings, metadata, kwargs, file and line offset), provider/model, and actual schema fingerprint. Changed findings can therefore reuse stale meta-analysis and suppress new issues. Key the exact request plus model/schema and add invalidation tests.
There was a problem hiding this comment.
Fixed in 9230576. Key now covers the fully-rendered prompt (rendered before the cache check, not after), the model, and a hash of the actual schema content (not just its class name). Added invalidation tests: findings-only prompt changes, different models, and different schemas all correctly miss.
|
|
||
| # Auto-discover baseline if not explicitly given | ||
| effective_baseline = baseline | ||
| if effective_baseline is None and not no_baseline: |
There was a problem hiding this comment.
Blocking: automatic discovery here trusts a .skillspector-baseline.yaml supplied by the scanned skill, so a malicious target can include a wildcard baseline and suppress its own findings by default. Baselines must be explicit caller-side policy, or stored and resolved outside the untrusted root; do not auto-load them from the target.
There was a problem hiding this comment.
Fixed in bec17a6. Replaced --no-baseline with --auto-baseline, opt-in and default False. Auto-discovery from the scan target now requires explicit caller opt-in rather than being trusted by default.
| # Offensive security override: authorized tools get a context-aware recommendation | ||
| # rather than a blanket DO_NOT_INSTALL, regardless of score-based severity. | ||
| classification = state.get("skill_classification") | ||
| if classification == "offensive_security": |
There was a problem hiding this comment.
Blocking: skill_classification comes from target-controlled manifest content, so a malicious skill can self-label as offensive_security and replace a DO_NOT_INSTALL recommendation. Keep the computed security recommendation authoritative and expose untrusted classification separately unless the caller explicitly authorizes an override.
There was a problem hiding this comment.
Fixed in 07aa733. The offensive_security self-declared override now requires an explicit --trust-skill-classification flag (default False) in addition to the declared value; the computed recommendation stays authoritative otherwise. The raw self-declared classification is always surfaced separately via an unconditional skill_declared_classification JSON field.
| } | ||
| Path(output).write_text(json.dumps(combined, indent=2), encoding="utf-8") | ||
| console.print(f"[green]Combined report saved to:[/green] {output}") | ||
| elif output: |
There was a problem hiding this comment.
Blocking output regression: for recursive non-JSON scans with --output, this branch writes nothing to the requested path and prints reports instead. Current main writes the combined report file. Preserve that behavior and add a regression for Markdown/SARIF output paths.
There was a problem hiding this comment.
Verified already fixed by the origin/main merge (_scan_multi_skill writes per-skill sections to the requested file). Added the missing SARIF regression test in 045d106 (Markdown coverage came in via the merge).
Deleted dangling >>>>>>> origin/main marker lines from two test files that were left over from previous manual merge conflict resolution: - tests/nodes/analyzers/test_behavioral_ast.py (last line) - tests/nodes/analyzers/test_static_patterns.py (line 758) Both files now parse correctly and all 115 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tor-inside-agent-session # Conflicts: # README.md # docs/DEVELOPMENT.md # src/skillspector/nodes/analyzers/mcp_tool_poisoning.py # src/skillspector/nodes/analyzers/semantic_developer_intent.py # src/skillspector/nodes/analyzers/semantic_quality_policy.py # src/skillspector/nodes/analyzers/semantic_security_discovery.py # src/skillspector/nodes/meta_analyzer.py # src/skillspector/nodes/report.py # src/skillspector/providers/__init__.py # tests/nodes/test_meta_analyzer.py # tests/unit/test_cli.py
…tput LLMAnalyzerBase.run_batches/arun_batches invoke the structured-output runnable with a bare string prompt, but RunnableLambda.invoke() (unlike BaseChatModel.invoke()) does no str-to-messages coercion. The closures in with_structured_output() were iterating the string character by character, so the JSON-schema instruction never got appended. Add _normalize_to_messages() to coerce str/BaseMessage/list/message-like inputs into a message list before augmenting with the schema instruction, and wire it into both the dict-schema and Pydantic-schema closures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Nesting the cache inside skill_dir let a malicious skill pre-seed the db to force cache hits with attacker-chosen LLM responses, or symlink the cache dir/file to escape the scan root. Cache now lives under a trusted, hashed, per-skill path under the OS app-cache root (LOCALAPPDATA on Windows, XDG_CACHE_HOME/~/.cache elsewhere), and LLMResponseCache._connect() refuses to operate on a symlinked cache dir or db file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ocuments test_default_cache_dir_never_under_skill_dir_when_skill_dir_is_cache_root set skill_dir to a subdirectory of the fake cache root, not the root itself, so it passed without ever exercising the documented containment gap. Point skill_dir at the fake cache root directly so the test genuinely reproduces the known, accepted limitation, and mark it xfail(strict=True) so it fails loudly if the gap is ever silently "fixed" without updating this test.
_cache_key(batch) previously hashed only batch.content + the static base_prompt string + response_schema.__name__, ignoring the fully-rendered prompt (which a subclass's build_prompt override can fold extra data like batch.findings into) and the model name. A changed finding set or a model switch could silently reuse a stale cached response generated for different inputs. - _schema_version now hashes the actual JSON schema instead of just the class name, so schema changes with the same class name also invalidate the cache. - _cache_key now takes the rendered prompt string, keyed with the model name and schema hash, instead of the Batch. - run_batches/arun_batches render the prompt once, before the cache check, and reuse it for both the cache key and the LLM call. Adds TestCacheKeyInvalidation covering: baseline cache hit on identical calls, cache miss when a subclass folds differing findings into the rendered prompt, cache miss across different models, and cache miss across different response schemas.
.skillspector-baseline.yaml found in the scanned directory was auto-loaded by default (skippable only with --no-baseline). Since the scanned directory can be attacker-controlled, a malicious skill could ship a baseline that suppresses findings about itself. Replace --no-baseline with --auto-baseline (default False) so auto-discovery is opt-in instead of opt-out.
…rust skill_classification is read from the scanned skill's own manifest, i.e. it is attacker-controlled: a malicious skill could label itself "offensive_security" purely to suppress a DO_NOT_INSTALL verdict. Trusting that self-declaration to override risk_recommendation is now gated behind a new trust_skill_classification state flag / --trust-skill-classification CLI flag (default False). JSON output always exposes the raw self-declared value as skill_declared_classification, independent of whether it was trusted, so it stays visible for review either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 8 verification: post-Task-2-merge, _scan_multi_skill's elif output: branch already builds per-skill sections via _result_body() and writes them with Path(output).write_text(...) — the markdown test for this was already present. This adds the missing SARIF-format counterpart so both non-JSON output formats are covered against the old regression where --output silently printed to console instead of writing the file. Confirmed by temporarily swapping in the pre-merge cli.py (db8235c): both the existing markdown test and this new SARIF test fail (no file written, content leaks to stdout instead) against the buggy version, and pass against current HEAD.
Mechanical only: import sorting, unused variable removal, and formatting. No behavioral changes; full suite (1344 passed) verified before and after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed
Re-requesting review — thanks for the thorough pass! |
What this PR does
The best time to scan a skill is the moment you're deciding whether to install it — which is usually inside an agent session (Claude Code, Cursor, Copilot, etc.), not before one. Until now, SkillSpector's LLM analysis required a separate API key configured outside the session, making it impractical to use the full scanner from within the same environment you're already working in.
This PR fixes that. With
SKILLSPECTOR_PROVIDER=subprocess, you point SkillSpector at any AI CLI already available in your session —claude -p,antigravity ask, or similar — and it uses that as the LLM backend. No additional API key. No separate authentication. If you're already in an agent session, you can run a full LLM-powered scan right there.Changes
New: subprocess provider
Run SkillSpector's LLM analysis from within your existing agent session — no separate API key required.
SubprocessProvider/SubprocessChatModelpipes each prompt to the configured shell command via stdin and reads the response from stdout. Works with any CLI-based AI:claude -p,antigravity ask,openclaw chat, or a custom wrapper script. Windows-compatible, with clear error messages when the command is missing or exits non-zero.New: SQLite LLM response cache
LLMResponseCachepersists LLM responses keyed by content hash to.skillspector-cache.db. Re-scanning the same skill content skips redundant LLM calls entirely. Particularly useful with the subprocess provider, where each call has visible latency in the session.New: scan cost controls
--skip-meta— bypass the meta-analyzer LLM pass for fast iterative scanning. Cuts token cost ~40–60% at the cost of more false positives. Recommended during development; drop it for final/CI runs.SKILLSPECTOR_META_BATCH_SIZE— tune findings-per-LLM-call without touching code.New: multi-skill scanning
--recursive— scan a directory of skills independently, with per-skill risk scores and a combined summary.--depth N— control nesting depth (default: 1).--detail— include full finding objects in recursive JSON output.New: test-fixture heuristics
AST4 and PE3 findings that match safe test-harness patterns are automatically downgraded to low confidence rather than flagged at full severity.
--include-test-fixturesopts out when you want to audit test code at full sensitivity.Other improvements
offensive_securityclassification — skills that declare this skip the score-based DO NOT INSTALL recommendation (security tools are expected to contain threat signatures)..skillspector-baseline.yamlin the target directory;--no-baselineto opt out.skillspector baselinewrites to the target directory by default rather than CWD.Test plan
uv run pytest— full suite passes with no regressionsskillspector scan ./my-skill/ --no-llmmatches output from mainSKILLSPECTOR_PROVIDER=subprocess SKILLSPECTOR_LLM_COMMAND="claude -p" skillspector scan ./my-skill/completes full LLM analysis without a separate API key--skip-metavisibly skips the meta-analyzer pass and runs faster.skillspector-cache.dbpresent, no LLM calls observed)skillspector scan ./skills-dir/ --recursiveproduces independent per-skill results with a combined summaryskillspector baselinewrites the baseline file into the scanned directory, not CWD🤖 Generated with Claude Code